Skip to content

fix: remove problematic defaults from sample_last_cit() parameters #2441

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Draft
wants to merge 7 commits into
base: main
Choose a base branch
from
Draft

fix: remove problematic defaults from sample_last_cit() parameters #2441

wants to merge 7 commits into from
+87 −44

Conversation

Copy link
Contributor

Copilot AI commented Oct 26, 2025 •
edited
Loading

The sample_last_cit() function had defaults agebins = n / 7100 and pref = (1:(agebins + 1))^-3 that only work for n ≥ 7100, causing silent failures with confusing error messages for smaller graphs.

Changes

  • Removed defaults for agebins and pref parameters in sample_last_cit() only
  • Preserved deprecated function: The deprecated lastcit.game() function retains its original default parameters to maintain backward compatibility (per project guideline: "Never change deprecated functions")
  • Enhanced parameter documentation explaining:
    • agebins must be ≥ 1 and controls aging discretization
    • pref should be a vector of length agebins + 1
    • Power-law decay (1:(agebins + 1))^-3 is a typical choice
  • Added examples demonstrating proper usage
  • Added tests for parameter validation and various configurations

Usage

The new function now requires explicit parameters:

# t^-3 power-law decay with 5 age bins
g <- sample_last_cit(100, edges = 1, agebins = 5, pref = (1:6)^-3)

The deprecated function maintains backward compatibility:

# Deprecated function still works with defaults for n ≥ 7100
g_old <- lastcit.game(7100)  # Still works

Fixes #1776

Original prompt

This section details on the original issue you should resolve

<issue_title>Default parameters of sample_last_cit not well documented</issue_title>
<issue_description>Unclear how agebins = n / 7100, pref = (1:(agebins + 1))^-3 are determined
The function only works if agebins>1, so by default it needs 7100 nodes. </issue_description>

Comments on the Issue (you are @copilot in this section)

@szhorvat This is a legacy function, probably motivated by Gábor's thesis work. I don't know how much it is used in practice ...

I wouldn't put too much effort into it.

IMO reasonable options are:

  • Add one to the agebins parameter;
  • Or just remove defaults entirely, and suggest the $t^{-3}$ decay function in the documentation (phrased as an example).

Any opinion @ntamas ? I see this one is not even exposed in Python.</comment_new>
<comment_new>@schochastics
Just stumbled upon it when cleaning games.R and its tests. I think your suggestions are reasonable but I will have at least a short look into the literature cause I am also not familiar with this</comment_new>

💬 We'd love your input! Share your thoughts on Copilot coding agent in our 2 minute survey.

Copilot AI changed the title [WIP] Document default parameters of sample_last_cit function fix: remove problematic defaults from sample_last_cit() parameters Oct 26, 2025
Copilot AI requested a review from krlmlr October 26, 2025 21:48
Copilot finished work on behalf of krlmlr October 26, 2025 21:48
krlmlr
krlmlr requested changes Oct 26, 2025
View reviewed changes
Copy link
Contributor

@krlmlr krlmlr left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@copilot: Merge main, resolve conflicts as needed, and obey updated instructions.

Copy link
Contributor Author

Copilot AI commented Oct 26, 2025

@copilot: Merge main, resolve conflicts as needed, and obey updated instructions.

Merged main in eb0808d. Also reverted changes to the deprecated lastcit.game() function per the updated "Never change deprecated functions" instruction in b6ed4d3. The new sample_last_cit() now requires explicit agebins and pref parameters while the deprecated function maintains backward compatibility with its original defaults.

Copilot AI requested a review from krlmlr October 26, 2025 23:05
Copilot finished work on behalf of krlmlr October 26, 2025 23:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@krlmlr krlmlr Awaiting requested review from krlmlr

Requested changes must be addressed to merge this pull request.

Assignees

@krlmlr krlmlr

Copilot code review Copilot

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Default parameters of sample_last_cit not well documented

2 participants

@krlmlr